Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix twitter not embedding in the first place (fxtwitter). Add support for tiktok (vxtiktok), reddit (fxreddit/rxddit), and threads (vxthreads) #12

Open
wants to merge 11 commits into
base: dpy2
Choose a base branch
from

Conversation

DoctorDinosaur
Copy link

@DoctorDinosaur DoctorDinosaur commented Nov 12, 2023

Closes #11

  • Add support for tiktok.com and vm.tiktok.com links; via vxtiktok.com
  • Twitter no longer embeds in the first place, so this cog didn't run for it. Changed that logic, and added support for x.com

(My new twitter code sucks ass, please review it. doing a str.split(), then running regex on each string. and excluding anything with an @ just incase some @everyone method exists. i dont code for discord lol)

@DoctorDinosaur
Copy link
Author

Also closes #13

@DoctorDinosaur
Copy link
Author

DoctorDinosaur commented Nov 12, 2023

68e754e adds support for https://github.com/MinnDevelopment/fxreddit and https://github.com/everettsouthwick/vxThreads

It also fixes some regex, the "."s in URLs weren't escaped. So, i.e., "instagramZcom" would have triggered the bot

@DoctorDinosaur DoctorDinosaur changed the title Add support for tiktok (vxtiktok). Fix twitter not embedding in the first place (fxtwitter) Fix twitter not embedding in the first place (fxtwitter). Add support for tiktok (vxtiktok), reddit (fxreddit/rxddit), and threads (vxthreads) Nov 12, 2023
@DoctorDinosaur
Copy link
Author

DoctorDinosaur commented Nov 12, 2023

Sometimes it fails to notice/fix a link. Reposting usually works. Not sure if thats lag on my test bot or what.

At any rate, the code is a bit of a mess with 3 more servivces, might need restructuring.

@DoctorDinosaur
Copy link
Author

Sometimes it fails to notice/fix a link. Reposting usually works. Not sure if thats lag on my test bot or what.

At any rate, the code is a bit of a mess with 3 more servivces, might need restructuring.

CORRECTION: It doesn't work if it takes a while for discord to add the original embed (i.e. for it to load the link + gen the embed)

Not sure how to fix that, beyond switching all the logic from looking for embeds, to just any url whether its embeded or not (like I've had to do for twitter)

@DoctorDinosaur
Copy link
Author

04b07ff seems going to on_raw_message_edit and restructuring around that fixes #14
Buggy mess tho, again pls review

@Injabie3 Injabie3 requested a review from quachtridat November 12, 2023 04:57
@quachtridat
Copy link
Collaborator

quachtridat commented Nov 14, 2023

At any rate, the code is a bit of a mess with 3 more servivces, might need restructuring.

Yes. This PR should be split into smaller ones. I imagine something like this:

  • Adjust the existing code w.r.t. Twitter.
  • Add support for X.
  • Add support for TikTok.
  • Add support for Reddit.
  • Add support for Threads.

One PR can be created each of the above. Preferably there's an issue attached to each "add support for something" kind of PR because then you can state why it is necessary and have a separate room for discussions there, but as I see it, if you are willing to implement and test all of this anyway, then some SNS additions to the cog don't sound that bad.

Having small PRs as I described will make it easier to:

  • Review code.
  • Do manual test runs for a PR (especially when the PR gets re-iterated for fixes/changes).
  • Discuss things that are relevant to only one SNS.

In PR posts, let us know what areas of code you want extra review, like you're already doing now. Make sure that if you're doing manual testing (which is likely what you'll have to do for this cog for now anyway), then be sure to include images/videos of the tests that you've done. Also, note anything that is specific to a(n) SNS when you do it. If something in the existing code should be made configurable (e.g., the follow-up messages that the bot posts after detecting a fixable link), do bring it up for discussions as well.

We will also need to adjust the casing of names in this cog's code because we want camelCase rather than snake_case, but that can be done later anyways.

@DoctorDinosaur
Copy link
Author

DoctorDinosaur commented Nov 14, 2023

Yes. This PR should be split into smaller ones.

Yeah ngl I wanted to split them but I only use github for personal projects (made this fork anyway to use for my own server) so didn't know a good way to split all the commits for different pulls after the fact; plus was worried about merges when I've made changes to the base code which they all rely on.

I've tested each service, they're all successfully replacing the links now.
And switching to raw_edit detection has fixed the issue with them not being picked up in the first place cos discord's embed server is slow to add the embeds for new/unseen links.

If you do still want seperate PRs, am I gonna need to refork, make new branches for each, then port over the code? Or is there a better way. I'll have a look when I get home.

@DoctorDinosaur
Copy link
Author

DoctorDinosaur commented Nov 14, 2023

What I meant by "code needs restructuring" is that, with 5 different services, there's a lot of repetition now.

I reckon at this rate a single handler applying multiple regex search/replaces would be more efficient.

Perhaps split into a text content replacer (twitter) and an embed replacer (everything else)

@quachtridat
Copy link
Collaborator

What I meant by "code needs restructuring" is that, with 5 different services, there's a lot of repetition now.

I reckon at this rate a single handler applying multiple regex search/replaces would be more efficient.

Perhaps split into a text content replacer (twitter) and an embed replacer (everything else)

I agree that we can do some refactoring to reduce code repetition, so I'd say if you're down to make the change, feel free to do a PR after adding support for these individual SNSes.

Yeah ngl I wanted to split them but I only use github for personal projects (made this fork anyway to use for my own server) so didn't know a good way to split all the commits for different pulls after the fact; plus was worried about merges when I've made changes to the base code which they all rely on.

If you do still want seperate PRs, am I gonna need to refork, make new branches for each, then port over the code? Or is there a better way. I'll have a look when I get home.

I suggest that in whatever the fork that you wanna use to do PRs, for each action item (that eventually will become a PR), make a branch. Let's say for the change where you wanna refactor the existing code, do a branch that branches from origin's dpy2. Then, when you wanna do something that requires the refactoring work you did in that branch (e.g., adding support for TikTok), you can make another branch off of that branch, then work there. After you've got the work done, you can start some PRs with those specific branches.

To flexibly work with local changes and branches, you need to learn commit rebasing. I think you only need to know basic rebasing (e.g., rebasing branch B on branch A after making additional commits in branch A, assuming B depends on A), but if you can organize your work well in advance, you won't even have to do that.

Also, because in this PR, you can see all the changes you wanted to do, you should have an idea of what code you should put in a smaller PR anyway. If you work on a branch that is about refactoring the existing code, then just take the specific portion of code from this PR to the branch in your Git workspace, then push it and form a PR with it. After that, move on to the ones that add support for various SNSes.

Code review will be resumed when the splitting task is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SNSConverter] Add vxtiktok
2 participants